@elupus: Note that the auto-enabling not working, is a regression from previous behaviour as this used to work fine until the previous round of subtitle-changes went in.

Well. I doubt it's possible to solve without breaking something else.

We are miss-using video settings for current status.

Either we:

• update gui with what the current status of the player is - > saved for
  next play.
• we don't update gui, so we can re-calc next time we open.

With current impl. those are our choices. :(

Taken from #4116

again.. you are missing to ALWAYS call SetSubtitleVisibleInternal. Now it's only called on a successful open. Can you please tell me what it is i'm missing to explain?

The call to SetSubtitleVisibleInternal should be after the if(!valid) check with parameters depending on what we decided above.

Thanks for the friendly reply.

You didn't reply for a week, but if I don't reply within some hours you open a pr that replaces mine?

Taken from your pr description:

It does not fix auto enable of added external subtitle after first play.

My pr did.

We must avoid all modifications of video player settings if no user action has been taken to support that.

My pr avoided that, yours does not! This can be seen here:

  if(!valid)
  {
    CloseSubtitleStream(true);
    visible = false;
  }
  SetSubtitleVisibleInternal(visible);

If a user has enables subs by default and the video has no subtitle stream, you will store false in the db. That is the reason why I only called it after a successful open(). And wrote that several times. This also fixes @arnova's issue.

I really did not intend to offend you by opening a new pull request. Sorry!

I just ended up working on it since you had ignored my comments on always
calling the Set..Internal, and gave no explanation as to why.

Like I said above. It's a tradeof between two issues. Either gui represent
player status or we support the case of external subs added later auto
enabling.

You solution caused gui to show enabled with no subs at all, to solve the
added later feature. IMHO that is not better.

Again I'm really sorry if I offended you.

Also note. that what you proposed would be inconsistent. Take the situation of one internal forced sub, prefer external subs on:

On first play this will get enabled and set visible. It will be stored in video settings.
On second play, external subs have been added, but will not be selected due to above.

If external subs was there on first play they would have been selected.

Auto enabling external subtitles have been working in xbmc since I can remember (definitely back to xbox days). The only difference now is that users have an option to prefer them or not. Previously, before ace2002 redid subtitle preference logic, external subtitles were preferred and enabled automatically. Not auto enabling them is definitely new and hopefully unexpected behaviour (when prefer external subtitles: on). If user sets Prefer external subtitles to on then external subtitles should be always preferred (displayed) no matter when they were actually added.

External subs will auto enable with this... It's only if you play the video
without any external subs, then added later that is the problem.

Note above used to work too. But can't be fixed without reverting bunch if
gui code.

I just ended up working on it since you had ignored my comments on always calling the Set..Internal, and gave no explanation as to why.

Of course I did, even several times. Last time (taken from #4116 (comment)) :

The call of SetSubtitleVisibleInternal() is at the end/latest possible place. Because if we don't have valid subs, e.g., the file has no subs at all, we decided to not alter the stored db value of subs on/off.

And that based on YOUR comment #4116 (comment) :

@elupus
If a user plays a video and doesn't modify any video settings manually, nothing should be stored.

I replied:

Then the whole auto selection/disabling of subs can't work.
With the current version of this pr we don't alter the settings in case no (valid) subtitles is available. But this has the effect of activated subs while there's no sub at all.

Follwed by your comment #4116 (comment) :

Well that would be due to using video settings to display current state of subtitles instead of the players state. Nothing i think we can solve for gotham.

But i don't think it's so bad that it shows as enabled even if there are none.

And here you wrote the contrary:

Like I said above. It's a tradeof between two issues. Either gui represent player status or we support the case of external subs added later auto enabling. You solution caused gui to show enabled with no subs at all, to solve the added later feature. IMHO that is not better.

You did not intend to offend me. Okay, I want to believe that, although it is quite hard give the facts I presented above.

External subs will auto enable with this... It's only if you play the video without any external subs, then added later that is the problem. Note above used to work too. But can't be fixed without reverting bunch if gui code.

That must be a long time ago the following is from the file dated back to 06.05.2012:

static bool PredicateSubtitlePriority(const SelectionStream& lh, const SelectionStream& rh)
{
  if(!g_settings.m_currentVideoSettings.m_SubtitleOn)
  {
    PREDICATE_RETURN(lh.flags & CDemuxStream::FLAG_FORCED
                   , rh.flags & CDemuxStream::FLAG_FORCED);
  }

  PREDICATE_RETURN(lh.type_index == g_settings.m_currentVideoSettings.m_SubtitleStream
                 , rh.type_index == g_settings.m_currentVideoSettings.m_SubtitleStream);

As you can see, if subs are on it takes the stream that was saved, and not a newly added ext. subtitle.

I meant very long ago yes. Before the predicate stuff was added.

Yes I indeed changed my mind on what is better gui or that feature.
Probably should have been clearer on that.

So the idea to ignore parameter has an ugly flaw.. Title ripped DVD without menu's should use normal logic.

@Voyager1 do you happen to remember the reason why you added the code to skip setting visibility of subs in OpenDefaultStreams for the navigator case in eee58b9 ?

@ace20022: There were some timeframes when it wasn't actually working.

I believe these are pretty common user scenarios:
1] check a video first for available subtitles, download subtitles externally from web, stop and start playback (usually faster than browsing for a subtitle) and play a video again.
2] play a video and skip through parts of it, decide to watch it later. Download external subtitles later before the actual full playback.
In both situations I'd expect to have an external subtitles displayed without any other required action.
I'm having hard time to find out a reason when user who've gone through the hassle of obtaining an external subtitle and chose to prefer external subtitles in GUI would actually not want to display them by default.
If xbmc is supposed (in some kind of scenario) to not actually prefer external subtitles when they are detected at the beginning of the playback and Prefer external subtitles setting is on, you can remove this setting from GUI completely (because it doesn't work) or rename it (that it reflects it only works on first playback). The best way would be imo to fix the current behavior rather than truncate a previously working feature.

Okey.. If having invalid GUI display is better in the no subtitles at all case, i can accept that. But it will still be invalid in the case of the video having internal subtitles and you open it once, then add external.

That is just not solvable at the moment sadly.

@elupus: Ok so if I understand correctly we at least agree that for movies that didn't have any subtitles before (-1 for the subtitle stream in the db), external subs added later on should always auto-enable (if subtitles are enabled in the default video settings) ?

I've restored the workaround for that special case. I've dropped the dvdplayer "fix" since it seems that will just make things worse.

@Voyager1 if you could test the case that was the source of: eee58b9 that would be very good.
@ace20022 this should now be almost identical to your last patch. Just that we always call SetSubtitleVisibleInternal at the end, and less code by adding relevant check to sorting.

jenkins build this please

Since I managed to get some junk in that last push and had lost correct author on one commit. Dear jenkins build this please

I only use external subs rarely, but I wouldn't want an external subtitle to be auto-enabled when I watch a movie. They only should be shown when a user manually turns on subtitles for the playing video like it's the case for embedded ones (unless they are forced ofc).
My usecase of external subs is mainly when I want to watch a movie in it's original language (en) but the video only has subs for my language (de) but I prefer the subs to be in same language as audio stream.

The setting "prefer external/downloaded over embedded subs" should only prefer the external subs over the embedded one if both are available for a certain language. It's no "always enforce external subs" setting.

If you don't want external subtitles to be automatically displayed it's reasonable to have Prefer external subtitles off and manually turn them on while playing a video when you need them to be displayed.

Your suggestion would actually not help your case, but would make others who use external subtitles exclusively pretty much difficult to select them manually all the time (over embedded subtitles with forced or default flags).

@elupus: Sorry in advance for maybe a "dumb" question. Do I understand correctly that the only problem with auto enabling external subtitles with your case (video having internal subtitles and you open it once, then add external) is because on first video playback xbmc saves Audio, Subtitles and Video settings to settings table in database for that file and you don't want to alter that once it saved without user interaction? If it is so why these settings are automatically saved when user didn't actually make any active change himself?

@ezechiel1917 The setting is called "prefer external subtitels". So in case I download a external sub for a language I also have an embedded sub for, the external should be preferred in the predication. That's at least how I understand this setting.
What you would want is a "force external subtitles" setting right? So whenever an external sub is present it should be used over any internal ones, regardless if it matches the preferred subtitle language or alike. And this is something completely different in my eyes.
edit: this doesn't mean that we shouldn't support it, but IMO that's not what this setting is supposed to do. So we probably need a spinner for "priority of downloaded subtitles" with options like "none", "prefer", "force". Where "prefer" is what I described above and "force" what you want.

Well it is kinda strange that we store SubtitleOn = 0 (false) in the db for a subtitle which isn't there (-1 in the db), this isn't how it always been. IMO it should reflect the default video settings which says it's enabled by default, even when there's no sub (-1) (yet).

@da-anda: That makes sense. Suggested spinner could also potentially help with elupus case if user deliberately choose Forced for External subtitles priority (Then external subtitles could be always displayed even over any previously saved SubtitleStream in settings table).

@arnova the problem is that "they way it used to be" has been broken by: f9fff03 which started using the video settings as what to display in the GUI. Now i fully see why that change was made. But it breaks the split between GUI display and stored settings.

@Voyager1 do you happen to remember the reason why you added the code to skip setting visibility of subs in OpenDefaultStreams for the navigator case in eee58b9 ?

The reason here was simple: we enter several times in OpenDefaultStreams - once initially, once after IsBetterStream... Problem is that during first pass, valid became false because the subtitle streams are not known yet - and thus the logic would turn off the subtitles visibility too early, even if the DVD navigator would at the second pass have subtitle streams (problem is that at the second pass there's nothing to turn them back on before finding valid subtitle streams)

@Voyager1 ok. That issue is sort of handled too by the special case commit i added. So we should be okey there.

@ezechiel1917 It's not a question of want. We can't alter it. The current code respect the selected video stream from previous run. So if we store video settings at all, we will always return to previous settings.

That issue is sort of handled too by the special case commit i added. So we should be okey there.

I tested it and indeed it works. I also looked at the commit and you mention it's a temporary fix. Let's just keep in mind to be careful when re-enabling it with a better fix, because the DVD use case will always be a little bit different.

@popcornmix can you check rbpi build with this, since jenkins isn't too happy about the ffmpeg changed.

OMXPlayer.cpp: In member function ‘void COMXPlayer::OpenDefaultStreams(bool)’:
OMXPlayer.cpp:873:81: error: conversion from ‘OMXSelectionStream’ to non-scalar type ‘OMXSelectionStreams {aka std::vector<OMXSelectionStream>}’ requested
OMXPlayer.cpp:874:36: error: ‘OMXSelectionStreams’ has no member named ‘language’
OMXPlayer.cpp:882:15: error: ‘class PredicateSubtitlePriority’ has no member named ‘relevant’
OMXPlayer.cpp: In member function ‘virtual void COMXPlayer::Process()’:
OMXPlayer.cpp:1085:34: warning: unused variable ‘nav’ [-Wunused-variable]

You've missed this from omxplayer patch:

+ PredicateSubtitleFilter filter;
...
+      filter(lang)

OMXSelectionStreams as = m_SelectionStreams.Get(STREAM_AUDIO, GetAudioStream());

should be

OMXSelectionStream as = m_SelectionStreams.Get(STREAM_AUDIO, GetAudioStream());

@elupus I see, but why is it needed to store video setting if user didn't change anything while playing a video? Wouldn't global default settings be sufficient in such case or is the goal here that once you play a video, to play it with exactly the same settings on next playback (including the case where user only pressed start/stop)?

IMO we should differ between resume state (which would hold the last used setting) and the default setting for a video. This would also fix this bug. So we'd need to store the audio and subtitle track on a per resume point basis for ongoing playbacks and start from defaults/stored settings on new playback.

for the record, this elupus@c6d477d is not my patch.

@ace20022 hmm.. okey who's is it? it had your name in your branch

@popcornmix there. let's hope that was it. btw, i have factored all this out into separate files to avoid the duplication for Gotham+1. -400 lines of code duplication :)

@elupus That builds okay.

okey who's is it? it had your name in your branch

No idea, but in the commit from my branch that looks quite similar, SetSubtitleVisibleInternal(bool bVisible); is private

Ah, I indeed squashed another commit into yours that moved it to protected.
Will restore my name.

that's even better

@elupus: Please wait for an RM to merge at this point unless the RM has specifically said it's OK in the PR. Thanks for flying Gotham airways. :)

@elupus slaps himself with a large trout.

Sorry i'm usually pretty good at that :(

On Sun, Feb 9, 2014 at 9:30 PM, jmarshallnz notifications@github.comwrote:

@elupus https://github.com/elupus: Please wait for an RM to merge at
this point unless the RM has specifically said it's OK in the PR. Thanks
for flying Gotham airways. :)

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4176#issuecomment-34585354
.